feat(memory): add relation graph for conflict detection and retrieval#708
feat(memory): add relation graph for conflict detection and retrieval#708mvanhorn wants to merge 3 commits intovolcengine:mainfrom
Conversation
Add a lightweight typed relation model between memories to enable
conflict detection during dedup, supersession tracking, and
related-context retrieval.
Changes:
- New MemoryRelationStore with CRUD operations and supersession queries
- New RelationType enum: supersedes, contradicts, related_to, derived_from
- MemoryDeduplicator now creates supersedes/contradicts relations on
MERGE/DELETE decisions when a relation store is provided
- HierarchicalRetriever supports follow_relations parameter to filter
superseded memories and surface related context
- New API endpoint GET /memories/{uri}/relations for relation queries
- Collection schema for future VikingDB persistence
- Unit tests for relation store CRUD and dedup integration
Refs: volcengine#269
qin-ctx
left a comment
There was a problem hiding this comment.
Review Summary
This PR introduces a typed memory relation graph (supersedes, contradicts, related_to, derived_from) to enable conflict detection during dedup and retrieval filtering. The data model and store API are well-designed, but two issues need attention before merging:
- Bug: The
supersedesrelation direction is inverted for MERGE decisions — the surviving (updated) memory gets marked as superseded, causing it to be filtered out during retrieval. - Incomplete wiring: The relation store, router, dedup integration, and retriever integration are all defined but never connected in service initialization, making the feature dead code.
Additionally, ~50% of changed files contain only ruff formatting changes unrelated to the feature. Consider splitting those into a separate PR.
| ) | ||
|
|
||
| for action in result.actions: | ||
| if action.decision == MemoryActionDecision.MERGE: |
There was a problem hiding this comment.
[Bug] (blocking) The supersedes relation direction is inverted for MERGE decisions.
In the MERGE flow, the candidate memory is not created (decision=NONE). Its content is merged into the existing memory, which survives with updated content. However, this code creates:
pending://... --supersedes--> existing_memory_uri
When HierarchicalRetriever._apply_memory_relations() later calls is_superseded(existing_uri), it returns True because there's a supersedes relation targeting the existing URI. This causes the surviving, updated memory to be filtered out of retrieval results.
The PR description's example assumes MERGE produces two distinct URIs (theme_001 and theme_002), but the actual code flow is:
- Candidate has no URI → falls back to
pending://scheme - Existing memory keeps its URI and gets updated content
- The relation incorrectly marks the kept memory as superseded
For MERGE, either:
- Don't create a
supersedesrelation (MERGE is an update, not a replacement), or - Use a different relation type (e.g.,
derived_from) to record the merge audit trail without affecting retrieval filtering
| from openviking.server.models import MemoryRelationListResponse, MemoryRelationResponse, Response | ||
| from openviking.storage.memory_relation_store import MemoryRelationStore, RelationType | ||
|
|
||
| router = APIRouter(prefix="/api/v1/memories", tags=["memory-relations"]) |
There was a problem hiding this comment.
[Design] (blocking) The entire feature is dead code — nothing is wired up.
- This router is never registered in
app.py(app.include_routernot called, not added torouters/__init__.py) MemoryRelationStoreis never instantiated during service startupMemoryDeduplicatorinstantiation incompressor.py:68doesn't passrelation_storeHierarchicalRetrieverinstantiation inviking_fs.py:647,792doesn't passmemory_relation_store
Please either:
- Add the wiring code (service init → store creation → inject into dedup/retriever/router), or
- Explicitly document this as Phase 1 (data model only) with a follow-up issue for the wiring
| _ctx: RequestContext = Depends(get_request_context), | ||
| ) -> Response: | ||
| """Get typed relations for a memory URI. | ||
|
|
There was a problem hiding this comment.
[Suggestion] (non-blocking) RelationType(type) raises ValueError for invalid enum values, resulting in a 500 instead of 400.
Also, the direction parameter accepts any string without validation — passing direction="invalid" silently returns empty results.
Consider:
try:
relation_type = RelationType(type) if type else None
except ValueError:
return Response(status="error", error=ErrorInfo(code="INVALID_ARGUMENT", message=f"Invalid relation type: {type}"))
if direction not in ("outgoing", "incoming", "both"):
return Response(status="error", error=ErrorInfo(code="INVALID_ARGUMENT", message=f"Invalid direction: {direction}"))Or use FastAPI's enum parameter validation by defining direction as a Literal["outgoing", "incoming", "both"].
| {"FieldName": "target_uri", "FieldType": "string"}, | ||
| {"FieldName": "relation_type", "FieldType": "string"}, | ||
| {"FieldName": "created_at", "FieldType": "date_time"}, | ||
| {"FieldName": "metadata", "FieldType": "string"}, |
There was a problem hiding this comment.
[Suggestion] (non-blocking) The schema defines account_id but MemoryRelation dataclass has no corresponding field. When you implement VikingDB persistence, this mismatch will need to be resolved — either add account_id to the dataclass or remove it from the schema.
| results.sort(key=lambda x: x.score, reverse=True) | ||
| return results | ||
|
|
||
| async def _apply_memory_relations(self, matched: List[MatchedContext]) -> List[MatchedContext]: |
There was a problem hiding this comment.
[Suggestion] (non-blocking) _apply_memory_relations has no test coverage. Given the supersedes direction bug identified in _record_dedup_relations, an integration test that exercises the full dedup → retrieval pipeline would catch this class of issues.
Consider adding a test that:
- Creates a MERGE dedup result and records relations
- Runs
_apply_memory_relationson a result set containing the merged memory - Asserts the merged memory is not filtered out
…dd validation - Fix inverted supersedes direction in MERGE dedup: the surviving (existing) memory now supersedes the candidate, not the other way around. Previously, _apply_memory_relations would filter out the surviving memory during retrieval. - Wire up the full feature: - Register memory_relations router in __init__.py and app.py - Pass MemoryRelationStore to MemoryDeduplicator via compressor.py - Pass memory_relation_store to HierarchicalRetriever via viking_fs.py - Add input validation in memory_relations router: - RelationType enum validation returns 400 instead of 500 - Direction parameter validation - Add account_id field to MemoryRelation dataclass to match the VikingDB collection schema Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed all feedback in 74a93b4:
|
|
Addressed all feedback in 74a93b4:
Re: integration test coverage for the dedup-to-retrieval pipeline - agreed that would strengthen confidence. Happy to add in a follow-up. |
Summary
supersedes,contradicts,related_to,derived_from) for conflict detection, supersession tracking, and related-context retrievalsupersedesrelations, DELETE decisions producecontradictsrelationsHierarchicalRetriever.retrieve()with an optionalfollow_relationsparameter that filters superseded memories and surfaces related contextGET /memories/{uri}/relationsAPI endpoint for querying memory relationsMotivation
OpenViking's memory deduplicator operates on vector similarity alone. When memories contradict each other (e.g., "user prefers dark mode" vs "user switched to light mode"), the system has no structured way to track the relationship between them. In #269, @qin-ctx noted that "relation table capability hasn't been built" when discussing how to clean up superseded memories.
This affects three areas:
Example flow
Changes
openviking/storage/memory_relation_store.pyMemoryRelationStorewith CRUD,RelationTypeenum,MemoryRelationdataclassopenviking/storage/collection_schemas.pymemory_relation_collectionschema for future VikingDB persistenceopenviking/session/memory_deduplicator.pyrelation_store, create relations on MERGE/DELETEopenviking/retrieve/hierarchical_retriever.pymemory_relation_store, addfollow_relationsparam, filter supersededopenviking/server/models.pyMemoryRelationResponseandMemoryRelationListResponseopenviking/server/routers/memory_relations.pyGET /memories/{uri}/relationsendpointtests/unit/storage/test_relation_store.pytests/unit/session/test_dedup_relations.pyEvidence
MAX_RELATIONS = 5constant defined but unused for memory relationsThe
MAX_RELATIONS = 5constant already exists inhierarchical_retriever.pybut is only used for resource relations, not memory relations. This PR extends the concept to the memory layer, where qin-ctx identified the gap in #269.Alternatives considered
Embedding relations in Context metadata fields. Simpler but doesn't support bidirectional queries or the kind of cleanup described in #269.
Test plan
pytest tests/unit/storage/test_relation_store.py- CRUD operations, dedup, deletion, round-trip serializationpytest tests/unit/session/test_dedup_relations.py- MERGE creates supersedes, DELETE creates contradicts, no-store no-crash, skip creates nothingrelation_storeparameter is optional everywhere (backward compatible)Generated with Claude Code | Refs: #269